-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
caddyauth: Speed up basicauth provision, deprecate scrypt
#4720
Conversation
161d6f6
to
fd96647
Compare
I decided to make the commit to deprecate This is a big UX simplification and avoids a lot of potential footguns that scrypt carries. |
scrypt
e6f83c8
to
da828d2
Compare
da828d2
to
f45baa4
Compare
@mholt can I bump this up your queue? I'd really like to get this in for 2.6.0 betas, since it involves a deprecation and a significant performance fix. |
Yeah. But events first :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a nit so I don't forget later to remove the cruft.
We'll document that scrypt is being deprecated and may be removed in the future -- not because it's broken, just because the config surface isn't ideal for our use case. Maybe I'll move it into a separate plugin repo later.
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
d2d103a
to
537125d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Thank you! Sorry it took me a while to get around to this.
When provisioning
basicauth
, there's a big performance hit on loading/reloading configs because it calculates a fake password; we don't need to do this on-the-fly, it's totally fine to hash the password once offline and just hardcode it.I did change the
Hasher
interface unfortunately because it's really the cleanest way to implement this, but I'm not aware of any plugins that implement another hashing algorithm anyways, so the breaking change should probably be fine.FWIW I still think we should drop
scrypt
altogether, and stop using base64-encoded strings.I don't see any compelling reason to keep
scrypt
, it has way too many knobs, and having the salt not being built-in makes it very error prone to use correctly. See https://passlib.readthedocs.io/en/stable/lib/passlib.hash.scrypt.html for example, which discourages its use.And no need for base64 because
bcrypt
already uses a safe string format (called Modular Crypt Format), so we're just base64 encoding an already-ASCII string which just obfuscates the configured work factor (which is plainly visible in the bcrypt header). For example, thebcrypt
I hard-coded in here, base64 decoded, is$2a$14$X3ulqf/iGxnf1k6oMZ.RZeJUoqI9PX2PM4rS5lkIKJXduLGXGPrt6
which you can clearly see at the start$14
which means the work factor is14
. That's its only config knob, and can tell you if it's time to re-hash the passwords if that work factor is now too weak (14 is still ridiculously strong, arguably way too high for a webserver, see https://twitter.com/TychoTithonus/status/1546270126730723329 and above).